Skip to content

CLID-290: feat: Add sparse manifest platform filtering for multi-arch images#1435

Open
aguidirh wants to merge 10 commits into
openshift:mainfrom
aguidirh:CLID-290
Open

CLID-290: feat: Add sparse manifest platform filtering for multi-arch images#1435
aguidirh wants to merge 10 commits into
openshift:mainfrom
aguidirh:CLID-290

Conversation

@aguidirh

@aguidirh aguidirh commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Description

This PR implements sparse manifest platform filtering for multi-architecture images in oc-mirror. Users can now specify OS/Architecture pairs to filter which platforms are mirrored, significantly reducing disk usage and bandwidth requirements.

The implementation brings the platform filtering capability from containers/image (containers/container-libs PR #656) to oc-mirror. It uses sparse manifests where the manifest list references all platforms but only the specified platforms have their actual layer blobs downloaded.

Platform filtering works across all image types:

  • Additional images
  • Operator catalogs
  • Helm charts
  • Release images (with backward compatibility for deprecated architectures field)

Github / Jira issue: CLID-290

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update on openshift docs

How Has This Been Tested?

Sparse Manifest Platform Filtering — Manual Test Plan

What the feature does

When platform.platforms is set in the ISC, oc-mirror copies only the requested
platform blobs from multi-arch manifest lists. The manifest list index in the target
registry is preserved intact, but only the specified architecture blobs are present.
This requires the target registry to have blob-presence validation disabled
(validation.manifests.indexes.platforms: none).


Prerequisites

1. Registries

Registry for filtered ISC run (isc-platforms.yaml)localhost:5000:

podman run -dt \
  -p 5000:5000 \
  -v /home/aguidi/go/src/github.com/aguidirh/oc-mirror/alex-tests/local-registry/registry-config.yml:/etc/distribution/config.yml:z \
  --name local-registry-with-sparse-manifest \
  docker.io/distribution/distribution:3

Registry for baseline ISC run (isc.yaml)localhost:6000:

podman run -dt \
  -p 6000:5000 \
  -v /home/aguidi/go/src/github.com/aguidirh/oc-mirror/alex-tests/local-registry/registry-config.yml:/etc/distribution/config.yml:z \
  --name local-registry-without-sparse-manifest \
  docker.io/distribution/distribution:3

Both registries use the same config (which includes platforms: none). The difference
is which ISC is mirrored to each one.

2. ISC files used

File Purpose
alex-tests/alex-isc/isc-platforms.yaml All image types with platforms field — primary test ISC
alex-tests/alex-isc/isc.yaml All image types without platforms — backward compat + comparison baseline

3. Verification script

chmod +x scripts/verify-platform-filter.sh

Test Suite


TEST 1 — New Platforms field: all image types in one run (M2M)

ISC: alex-tests/alex-isc/isc-platforms.yaml

./bin/oc-mirror --v2 \
  --config alex-tests/alex-isc/isc-platforms.yaml \
  docker://localhost:5000 \
  --workspace file://alex-tests/clid-290-platforms/working-dir \
  --dest-tls-verify=false

Expected log: Warning printed before copying:

WARN: Platform filtering with the Platforms field requires a registry with manifest
      list blob check disabled (e.g. validation.manifests.indexes.platforms: none).

Verify — report mode:

./scripts/verify-platform-filter.sh localhost:5000

What to look for in the report output:

Image Expected present Expected missing
openshift/release-images (top-level release payload) amd64, arm64, s390x ppc64le
openshift/release:4.18.1-multi-* (component images) amd64, arm64, s390x ppc64le
rh_ee_aguidi/multi-platform-container amd64, arm64 s390x, ppc64le
rh_ee_aguidi/empty-image all (no filter) nothing
stefanprodan/podinfo amd64 only arm64, others
projectsigstore/cosigned amd64, arm64 others
Operator images from v4.18 catalog amd64 arm64+
Operator images from v4.19 catalog amd64, arm64 others
Operator images from v4.20 catalog all (no filter) nothing
Operator images from v4.21 catalog amd64, arm64, s390x, ppc64le none

Both openshift/release-images (top-level manifest list from Cincinnati) and the
openshift/release component images (extracted from image-references) must show only
the configured platforms. The verify script checks both.

Why report mode only? Validate mode applies a global allowed list, but this ISC
uses per-image platform restrictions that can't be expressed as a single global set.


TEST 2 — Backward compat: old behavior unchanged (M2M)

ISC: alex-tests/alex-isc/isc.yaml (no platforms field anywhere)

./bin/oc-mirror --v2 \
  --config alex-tests/alex-isc/isc.yaml \
  docker://localhost:6000 \
  --workspace file://alex-tests/clid-290/working-dir \
  --dest-tls-verify=false

Expected:

  • No sparse manifest warning in logs
  • Release images: appear as single-manifest (no index) — Cincinnati called with
    ?arch=amd64, only single-arch amd64 payload
  • Non-release manifest-list images: all platform blobs present ([N present, 0 missing])

Verify:

./scripts/verify-platform-filter.sh localhost:6000

Cleanup between M2M and M2D runs

Before running M2D tests, free up disk space:

rm -rf alex-tests/clid-290-platforms/working-dir
rm -rf alex-tests/clid-290/working-dir

podman rm -f local-registry-with-sparse-manifest local-registry-without-sparse-manifest

podman run -dt \
  -p 5000:5000 \
  -v /home/aguidi/go/src/github.com/aguidirh/oc-mirror/alex-tests/local-registry/registry-config.yml:/etc/distribution/config.yml:z \
  --name local-registry-with-sparse-manifest \
  docker.io/distribution/distribution:3

podman run -dt \
  -p 6000:5000 \
  -v /home/aguidi/go/src/github.com/aguidirh/oc-mirror/alex-tests/local-registry/registry-config.yml:/etc/distribution/config.yml:z \
  --name local-registry-without-sparse-manifest \
  docker.io/distribution/distribution:3

TEST 3 — New Platforms field: M2D then D2M

Step 1 — M2D:

rm -rf ~/.oc-mirror
./bin/oc-mirror --v2 \
  --config alex-tests/alex-isc/isc-platforms.yaml \
  file://alex-tests/clid-290-platforms/m2d-output

Step 2 — Prepare for D2M (simulate disconnected environment):

rm -rf ~/.oc-mirror
mkdir -p alex-tests/clid-290-platforms/d2m-input
mv alex-tests/clid-290-platforms/m2d-output/mirror_000001.tar \
   alex-tests/clid-290-platforms/d2m-input/
rm -rf alex-tests/clid-290-platforms/m2d-output/working-dir

Step 3 — D2M:

./bin/oc-mirror --v2 \
  --config alex-tests/alex-isc/isc-platforms.yaml \
  --from file://alex-tests/clid-290-platforms/d2m-input \
  docker://localhost:5000 \
  --dest-tls-verify=false

Verify — same checks as TEST 1:

./scripts/verify-platform-filter.sh localhost:5000

Expected: Identical results to TEST 1. Platform filtering preserved through archive.
Check logs to confirm no external HTTP calls.


TEST 4 — Backward compat: M2D then D2M with isc.yaml

Step 1 — M2D:

rm -rf ~/.oc-mirror
./bin/oc-mirror --v2 \
  --config alex-tests/alex-isc/isc.yaml \
  file://alex-tests/clid-290/m2d-output

Step 2 — Prepare:

rm -rf ~/.oc-mirror
mkdir -p alex-tests/clid-290/d2m-input
mv alex-tests/clid-290/m2d-output/mirror_000001.tar \
   alex-tests/clid-290/d2m-input/
rm -rf alex-tests/clid-290/m2d-output/working-dir

Step 3 — D2M:

./bin/oc-mirror --v2 \
  --config alex-tests/alex-isc/isc.yaml \
  --from file://alex-tests/clid-290/d2m-input \
  docker://localhost:6000 \
  --dest-tls-verify=false

Verify:

./scripts/verify-platform-filter.sh localhost:6000

What to look for:

  • Release images: single-manifest (no index) — single-arch amd64
  • Non-release manifest-list images: [N present, 0 missing] — all platforms present

TEST 5 — Validation error: both Platforms and Architectures set

Create /tmp/invalid-isc.yaml:

kind: ImageSetConfiguration
apiVersion: mirror.openshift.io/v2alpha1
mirror:
  platform:
    platforms:
      - os: linux
        architecture: amd64
    architectures:
      - arm64
    channels:
      - name: stable-4.18
        minVersion: 4.18.1
        maxVersion: 4.18.1
./bin/oc-mirror --v2 --config /tmp/invalid-isc.yaml \
  docker://localhost:5000 \
  --workspace file:///tmp/test-invalid \
  --dest-tls-verify=false

Expected: oc-mirror exits immediately with:

invalid configuration: platform.platforms and platform.architectures cannot be used together

TEST 6 — Deprecated Architectures field with multi value

Create /tmp/isc-archs-multi.yaml:

kind: ImageSetConfiguration
apiVersion: mirror.openshift.io/v2alpha1
mirror:
  platform:
    architectures:
      - multi
      - amd64
  additionalImages:
    - name: quay.io/rh_ee_aguidi/multi-platform-container:latest
./bin/oc-mirror --v2 --config /tmp/isc-archs-multi.yaml \
  docker://localhost:6000 \
  --workspace file://alex-tests/tmp-archs-multi/working-dir \
  --dest-tls-verify=false

Expected:

  • Deprecation warning: platform.architectures is deprecated; use platform.platforms instead
  • Command exits successfully (no crash from multi in architectures)
  • multi-platform-container repo exists in :6000

Verify:

./scripts/verify-platform-filter.sh localhost:6000

TEST 7 — Negative: target registry without sparse manifest support

Start a plain registry (no platforms: none):

podman run -dt \
  -p 7000:5000 \
  --name local-registry-strict \
  docker.io/distribution/distribution:3
./bin/oc-mirror --v2 \
  --config alex-tests/alex-isc/isc-platforms.yaml \
  docker://localhost:7000 \
  --workspace file://alex-tests/tmp-strict/working-dir \
  --dest-tls-verify=false

Expected:

  • Sparse manifest warning printed at start
  • Mirror fails with registry error about missing blobs or manifest validation

Summary Table

# Workflow ISC Registry Pass condition
TEST 1 M2M isc-platforms.yaml :5000 Only declared arch blobs present per image
TEST 2 M2M isc.yaml :6000 Release: single-arch amd64; others: all blobs
TEST 3 M2D+D2M isc-platforms.yaml :5000 Same as TEST 1, no external HTTP calls
TEST 4 M2D+D2M isc.yaml :6000 Same as TEST 2, no external HTTP calls
TEST 5 invalid (both fields) Error before mirroring starts
TEST 6 M2M archs:[multi,amd64] :6000 Success, deprecation warning, no crash
TEST 7 M2M isc-platforms.yaml :7000 (strict) Fails with registry error

Known Limitations

  1. Release component images may remain single-arch. The top-level release image is
    a manifest list and is correctly filtered. Component images from image-references
    may still be single-arch depending on the OCP version.

  2. D2M can only copy what M2D stored. Requesting a platform in D2M that wasn't
    stored in M2D results in "no instances found" fallback — the whole image is copied.

  3. Single-arch operator/helm images. Many operator bundle images are published
    as single-arch (amd64 only). Platform filtering is a no-op for these; they are
    always copied whole via the fallback retry.

Script to verify archs on registries:

#!/usr/bin/env bash
# verify-platform-filter.sh
#
# Verify which platform blobs are actually stored in a registry for manifest-list images.
# Sparse manifest registries store the full index but may omit blobs for unselected platforms.
# This script distinguishes PRESENT (blob reachable) from MISSING (listed in index but no blob).
#
# Usage:
#   # Report all manifest-list images and their blob presence:
#   ./scripts/verify-platform-filter.sh localhost:6000
#
#   # Validate that no image has blobs outside the expected set (fail on unexpected extras only;
#   # single-arch images with fewer platforms than expected are fine):
#   ./scripts/verify-platform-filter.sh localhost:6000 linux/amd64 linux/arm64
#
#   # Side-by-side comparison of two registries (useful to spot the filtering effect):
#   ./scripts/verify-platform-filter.sh --compare localhost:5000 localhost:6000
#
#   # Side-by-side comparison AND validate the second registry against expected platforms:
#   ./scripts/verify-platform-filter.sh --compare localhost:5000 localhost:6000 linux/amd64 linux/arm64

set -euo pipefail

# ── colours ───────────────────────────────────────────────────────────────────
RED='\033[0;31m'; GREEN='\033[0;32m'; YELLOW='\033[1;33m'
CYAN='\033[0;36m'; BOLD='\033[1m'; RESET='\033[0m'

# ── helpers ───────────────────────────────────────────────────────────────────
die()  { echo -e "${RED}ERROR: $*${RESET}" >&2; exit 1; }
info() { echo -e "${CYAN}$*${RESET}"; }
ok()   { echo -e "    ${GREEN}✔ $*${RESET}"; }
fail() { echo -e "    ${RED}✘ $*${RESET}"; }
warn() { echo -e "    ${YELLOW}~ $*${RESET}"; }

require() { command -v "$1" &>/dev/null || die "$1 is required but not installed."; }
require curl
require python3

# ── registry helpers ──────────────────────────────────────────────────────────

# List all repositories in a registry.
list_repos() {
    curl -sf "http://${1}/v2/_catalog" \
        | python3 -c "import json,sys; [print(r) for r in json.load(sys.stdin).get('repositories',[])]"
}

# List non-signature tags for an image (skip *.sig tags, which are Cosign signatures).
list_tags() {
    local reg=$1 image=$2
    curl -sf "http://${reg}/v2/${image}/tags/list" \
        | python3 -c "
import json,sys
for t in (json.load(sys.stdin).get('tags') or []):
    if not t.endswith('.sig'):
        print(t)
"
}

# Fetch a manifest, accepting both manifest-list and single-manifest media types.
fetch_manifest() {
    local reg=$1 image=$2 ref=$3
    curl -sf \
        -H "Accept: application/vnd.docker.distribution.manifest.list.v2+json,application/vnd.oci.image.index.v1+json,application/vnd.docker.distribution.manifest.v2+json,application/vnd.oci.image.manifest.v1+json" \
        "http://${reg}/v2/${image}/manifests/${ref}"
}

# Return 0 if the manifest/blob for a digest is reachable (HTTP 200).
digest_present() {
    local reg=$1 image=$2 digest=$3
    local http_code
    http_code=$(curl -so /dev/null -w "%{http_code}" \
        -H "Accept: application/vnd.docker.distribution.manifest.v2+json,application/vnd.oci.image.manifest.v1+json" \
        "http://${reg}/v2/${image}/manifests/${digest}")
    [[ "$http_code" == "200" ]]
}

# Extract tab-separated "digest  os  arch  variant" lines from a manifest list JSON.
manifest_entries() {
    python3 -c "
import json,sys
m=json.load(sys.stdin)
for e in m.get('manifests',[]):
    p=e.get('platform',{})
    print(e['digest'] + '\t' + p.get('os','?') + '\t' + p.get('architecture','?') + '\t' + p.get('variant',''))
"
}

# ── check one image ───────────────────────────────────────────────────────────
#
# Prints a block like:
#   image:tag
#     ✔ linux/amd64
#     ✘ linux/s390x (blob missing)
#
# Sets globals: IS_INDEX, PRESENT_COUNT, MISSING_COUNT
# Returns 1 if any expected platform is missing or an unexpected one is present.
check_image() {
    local reg=$1 image=$2 tag=$3
    shift 3
    local -a expected_platforms=("$@")

    IS_INDEX=0 PRESENT_COUNT=0 MISSING_COUNT=0

    local manifest
    manifest=$(fetch_manifest "$reg" "$image" "$tag" 2>/dev/null) || return 0

    local has_manifests
    has_manifests=$(echo "$manifest" | python3 -c "import json,sys; print('yes' if 'manifests' in json.load(sys.stdin) else 'no')" 2>/dev/null)
    [[ "$has_manifests" == "yes" ]] || return 0   # single-manifest — nothing to check

    IS_INDEX=1

    # Collect per-platform results before printing so the header can come first.
    local -a lines=()
    local -a present_platforms=()

    while IFS=$'\t' read -r digest os arch variant; do
        local plat="${os}/${arch}"
        [[ -n "$variant" ]] && plat="${plat}/${variant}"

        if digest_present "$reg" "$image" "$digest"; then
            lines+=("ok:${plat}")
            present_platforms+=("$plat")
            (( PRESENT_COUNT++ )) || true
        else
            lines+=("fail:${plat}")
            (( MISSING_COUNT++ )) || true
        fi
    done < <(echo "$manifest" | manifest_entries)

    # Print header with summary counts.
    echo -e "  ${BOLD}${image}:${tag}${RESET}  [${PRESENT_COUNT} present, ${MISSING_COUNT} missing]"

    # Print per-platform lines.
    for line in "${lines[@]}"; do
        local status="${line%%:*}" plat="${line#*:}"
        if [[ "$status" == "ok" ]]; then
            ok "$plat"
        else
            fail "$plat (blob missing)"
        fi
    done

    # Validate against expected platforms if provided.
    # Only fails on UNEXPECTED platforms (present but not in the allowed set).
    # An image having fewer platforms than expected is fine — it may be single-arch.
    if (( ${#expected_platforms[@]} > 0 )); then
        local img_fail=0

        for got in "${present_platforms[@]}"; do
            local expected_flag=0
            for exp in "${expected_platforms[@]}"; do
                # Match exact or prefix (e.g. linux/arm64 matches linux/arm64/v8).
                [[ "$got" == "$exp" || "$got" == "${exp}/"* ]] && expected_flag=1 && break
            done
            (( expected_flag )) || { fail "UNEXPECTED platform present: ${got}"; img_fail=1; }
        done

        return $img_fail
    fi
}

# ── report mode ───────────────────────────────────────────────────────────────

report_registry() {
    local reg=$1
    shift
    local -a expected_platforms=("$@")
    local total_images=0 total_index=0 total_missing=0 total_fail=0

    info "\n=== Registry: ${reg} ==="
    if (( ${#expected_platforms[@]} > 0 )); then
        echo -e "  Expected platforms: ${BOLD}${expected_platforms[*]}${RESET}"
    fi
    echo ""

    while read -r repo; do
        while read -r tag; do
            (( total_images++ )) || true

            if ! check_image "$reg" "$repo" "$tag" "${expected_platforms[@]}"; then
                (( total_fail++ )) || true
            fi

            if (( IS_INDEX )); then
                (( total_index++ )) || true
                (( total_missing += MISSING_COUNT )) || true
            fi
        done < <(list_tags "$reg" "$repo")
    done < <(list_repos "$reg")

    echo ""
    if (( ${#expected_platforms[@]} > 0 )); then
        if (( total_fail == 0 )); then
            echo -e "${GREEN}${BOLD}PASS — all ${total_index} manifest list(s) match expected platforms${RESET}"
        else
            echo -e "${RED}${BOLD}FAIL — ${total_fail} image(s) have platform mismatches${RESET}"
            return 1
        fi
    else
        echo "Scanned ${total_images} image(s). ${total_index} are manifest lists, ${total_missing} missing blobs total."
    fi
}

# ── compare mode ─────────────────────────────────────────────────────────────

compare_registries() {
    local reg1=$1 reg2=$2
    shift 2
    local -a expected_platforms=("$@")

    info "\n=== Comparing ${reg1}  ↔  ${reg2} ==="
    if (( ${#expected_platforms[@]} > 0 )); then
        echo -e "  Validating ${reg2} against: ${BOLD}${expected_platforms[*]}${RESET}"
    fi
    echo ""

    local total=0 diff_count=0 fail_count=0

    # Helper: return newline-separated list of platforms whose blobs are present.
    get_present_platforms() {
        local reg=$1 image=$2 tag=$3
        local manifest
        manifest=$(fetch_manifest "$reg" "$image" "$tag" 2>/dev/null) || return 0
        echo "$manifest" | python3 -c "
import json,sys,urllib.request,urllib.error
m=json.load(sys.stdin)
if 'manifests' not in m: exit(0)
reg='${1}'; im='${2}'
for e in m.get('manifests',[]):
    p=e.get('platform',{})
    plat=p.get('os','?')+'/'+p.get('architecture','?')
    v=p.get('variant','')
    if v: plat+='/'+v
    try:
        req=urllib.request.Request(
            f'http://{reg}/v2/{im}/manifests/'+e['digest'],
            headers={'Accept':'application/vnd.docker.distribution.manifest.v2+json,application/vnd.oci.image.manifest.v1+json'})
        urllib.request.urlopen(req)
        print(plat)
    except urllib.error.HTTPError:
        pass
"
    }

    while read -r repo; do
        while read -r tag; do
            # Check if it's a manifest list in reg1.
            local manifest1
            manifest1=$(fetch_manifest "$reg1" "$repo" "$tag" 2>/dev/null) || continue
            local is_index
            is_index=$(echo "$manifest1" | python3 -c "import json,sys; print('yes' if 'manifests' in json.load(sys.stdin) else 'no')" 2>/dev/null)
            [[ "$is_index" == "yes" ]] || continue

            (( total++ )) || true
            echo -e "  ${BOLD}${repo}:${tag}${RESET}"

            local -a p1 p2
            mapfile -t p1 < <(get_present_platforms "$reg1" "$repo" "$tag")
            mapfile -t p2 < <(get_present_platforms "$reg2" "$repo" "$tag")

            # Union of all platforms across both registries.
            local -A all_plats=()
            for p in "${p1[@]}"; do all_plats["$p"]=1; done
            for p in "${p2[@]}"; do all_plats["$p"]=1; done

            local differs=0
            for plat in $(echo "${!all_plats[@]}" | tr ' ' '\n' | sort); do
                local in1=0 in2=0
                for p in "${p1[@]}"; do [[ "$p" == "$plat" ]] && in1=1; done
                for p in "${p2[@]}"; do [[ "$p" == "$plat" ]] && in2=1; done

                local r1_mark r2_mark
                (( in1 )) && r1_mark="${GREEN}✔${RESET}" || r1_mark="${YELLOW}—${RESET}"
                (( in2 )) && r2_mark="${GREEN}✔${RESET}" || r2_mark="${YELLOW}—${RESET}"

                echo -e "    ${plat}  (${reg1}: ${r1_mark}  ${reg2}: ${r2_mark})"
                (( in1 != in2 )) && differs=1
            done
            (( differs )) && (( diff_count++ )) || true

            # Validate reg2 against expected platforms if provided.
            # Only fails on UNEXPECTED platforms (present but not in the allowed set).
            if (( ${#expected_platforms[@]} > 0 )); then
                local img_fail=0
                for got in "${p2[@]}"; do
                    local expected_flag=0
                    for exp in "${expected_platforms[@]}"; do
                        [[ "$got" == "$exp" || "$got" == "${exp}/"* ]] && expected_flag=1 && break
                    done
                    (( expected_flag )) || { fail "${reg2} has UNEXPECTED: ${got}"; img_fail=1; }
                done
                (( img_fail )) && (( fail_count++ )) || true
            fi

            echo ""
        done < <(list_tags "$reg1" "$repo")
    done < <(list_repos "$reg1")

    echo "Scanned ${total} manifest list(s). ${diff_count} differ between registries."
    if (( ${#expected_platforms[@]} > 0 )); then
        if (( fail_count > 0 )); then
            echo -e "${RED}${BOLD}FAIL — ${fail_count} image(s) in ${reg2} don't match expected platforms${RESET}"
            return 1
        else
            echo -e "${GREEN}${BOLD}PASS — ${reg2} matches expected platforms on all manifest lists${RESET}"
        fi
    fi
}

# ── main ──────────────────────────────────────────────────────────────────────

usage() {
    grep '^# ' "$0" | sed 's/^# //' | grep -v '^$'
    exit 1
}

if [[ $# -eq 0 ]]; then usage; fi

if [[ "$1" == "--compare" ]]; then
    shift
    [[ $# -lt 2 ]] && die "--compare requires two registry arguments"
    reg1=$1; reg2=$2; shift 2
    compare_registries "$reg1" "$reg2" "$@"
else
    reg=$1; shift
    report_registry "$reg" "$@"
fi

Summary by CodeRabbit

  • New Features
    • Added platform selection support in configuration for operators, Helm charts, and additional images, enabling multi-architecture specification via OS/architecture pairs.
    • Platform filters are now propagated throughout the mirroring pipeline, ensuring per-image platform requirements are respected during mirror operations.

@openshift-ci
openshift-ci Bot requested review from adolfo-ab and r4f4 June 8, 2026 15:16
@openshift-ci

openshift-ci Bot commented Jun 8, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aguidirh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 8, 2026
@aguidirh aguidirh changed the title feat: Add sparse manifest platform filtering for multi-arch images WIP: CLID-290: feat: Add sparse manifest platform filtering for multi-arch images Jun 8, 2026
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jun 8, 2026
@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 8, 2026
@openshift-ci-robot

openshift-ci-robot commented Jun 8, 2026

Copy link
Copy Markdown

@aguidirh: This pull request references CLID-290 which is a valid jira issue.

Details

In response to this:

Description

This PR implements sparse manifest platform filtering for multi-architecture images in oc-mirror. Users can now specify OS/Architecture pairs to filter which platforms are mirrored, significantly reducing disk usage and bandwidth requirements.

The implementation brings the platform filtering capability from containers/image (containers/container-libs PR #656) to oc-mirror. It uses sparse manifests where the manifest list references all platforms but only the specified platforms have their actual layer blobs downloaded.

Platform filtering works across all image types:

  • Additional images
  • Operator catalogs
  • Helm charts
  • Release images (with backward compatibility for deprecated architectures field)

Github / Jira issue: CLID-290

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update on openshift docs

How Has This Been Tested?

Still in Progress.

Expected Outcome

Platform filtering reduces disk usage by 60-75% when mirroring only specific platforms instead of all architectures. Sparse manifests are created where manifest lists reference all platforms but only selected platforms have layer blobs downloaded.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@coderabbitai

coderabbitai Bot commented Jun 8, 2026

Copy link
Copy Markdown

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

This PR introduces per-image platform filtering across the oc-mirror pipeline. A new InstancePlatformFilter type and ConvertPlatformsToStringSlice utility are added. Platforms fields are added to config types (Platform, Operator, Chart, AdditionalImage) and internal types (RelatedImage, CopyImageSchema). The mirror engine's parseMultiArch and new determinePlatformSelection are updated to support comma-separated OS/ARCH platform lists. All collector paths (release, Helm, operator, additional images) and the batch worker are updated to propagate platform filters from configuration through to copy operations.

Changes

Per-image platform filtering across oc-mirror

Layer / File(s) Summary
Platform type, config, and internal struct contracts
internal/pkg/api/v2alpha1/type_platform.go, internal/pkg/api/v2alpha1/type_config.go, internal/pkg/api/v2alpha1/type_internal.go, internal/pkg/mirror/options.go
InstancePlatformFilter struct and ConvertPlatformsToStringSlice are added. Platforms []InstancePlatformFilter is added to Platform (with deprecated architectures documented), Operator, Chart, and AdditionalImage config types, along with Platform.DeepCopy() update. Platforms *[]string is added to RelatedImage and CopyImageSchema. CopyOptions.InstancePlatforms []string is added.
Mirror engine platform selection
internal/pkg/mirror/mirror.go, internal/pkg/mirror/mirror_test.go
determinePlatformSelection is added to prefer opts.InstancePlatforms over legacy --all/--multi-arch flags. parseMultiArch is updated to return both ImageListSelection and []InstancePlatformFilter, supporting comma-separated OS/ARCH input. copy() is wired to use the computed platform filters. Tests are updated for new error messages and comma-separated parse cases.
Operator collector platform propagation
internal/pkg/operator/interface.go, internal/pkg/operator/catalog_handler.go, internal/pkg/operator/filtered_collector.go, internal/pkg/operator/catalog_handler_test.go, internal/pkg/operator/filtered_collector_test.go
catalogHandlerInterface.getRelatedImagesFromCatalog gains a platforms *[]string parameter. CatalogHandler.getRelatedImagesFromCatalog and handleRelatedImages accept and assign platforms to each constructed RelatedImage. collectOperator converts op.Platforms and passes it through. Tests and mock are updated accordingly.
Release collector platform propagation
internal/pkg/release/local_stored_collector.go
getPlatformFilters() is added to derive platform filters from Platform.Platforms or deprecated Architectures. Mirror-based collection assigns filters to all RelatedImage entries. prepareM2DCopyBatch, prepareD2MCopyBatch, and all three handleGraphImage return paths propagate Platforms to CopyImageSchema.
Helm collector platform propagation
internal/pkg/helm/local_stored_collector.go
getImages gains a platforms parameter; M2D, D2M, and local chart collector call sites convert chart.Platforms and pass it. getImages assigns platforms to each discovered RelatedImage. prepareM2DCopyBatch and prepareD2MCopyBatch propagate RelatedImage.Platforms to CopyImageSchema.
Additional images collector and batch worker
internal/pkg/additional/local_stored_collector.go, internal/pkg/batch/concurrent_chan_worker.go
AdditionalImagesCollector converts img.Platforms and sets it on the appended CopyImageSchema. ChannelConcurrentBatch.Worker sets options.InstancePlatforms from img.Platforms before calling Mirror.Run.

Sequence Diagram(s)

sequenceDiagram
  participant Collector as Collector (Release/Helm/Operator/Additional)
  participant RelatedImage as RelatedImage
  participant BatchWorker as ChannelConcurrentBatch.Worker
  participant MirrorEngine as mirror.copy()
  participant determinePlatformSelection as determinePlatformSelection
  participant parseMultiArch as parseMultiArch

  Collector->>RelatedImage: sets Platforms *[]string from ConvertPlatformsToStringSlice
  Collector->>BatchWorker: enqueues CopyImageSchema with Platforms
  BatchWorker->>MirrorEngine: sets options.InstancePlatforms from img.Platforms
  MirrorEngine->>determinePlatformSelection: opts.InstancePlatforms or MultiArch/All
  determinePlatformSelection->>parseMultiArch: comma-separated OS/ARCH or legacy mode
  parseMultiArch-->>determinePlatformSelection: ImageListSelection + []InstancePlatformFilter
  determinePlatformSelection-->>MirrorEngine: imageListSelection + instancePlatforms
  MirrorEngine->>MirrorEngine: copy.Options{InstancePlatforms: instancePlatforms}
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

🚥 Pre-merge checks | ✅ 13 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 35.71% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Test Structure And Quality ⚠️ Warning PR tests use standard Go testing (not Ginkgo), making BeforeEach/AfterEach inapplicable. However, bare assertions like assert.NoError(t, err) without meaningful messages were found in catalog_han... Add context messages to all bare assertions: assert.NoError(t, err, "should handle related images") instead of assert.NoError(t, err), following Go testing best practices and existing message patterns in mirror_test.go.
✅ Passed checks (13 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed All test names in the modified test files (mirror_test.go, catalog_handler_test.go, filtered_collector_test.go) use static, hardcoded strings with no dynamic values (timestamps, UUIDs, pod names, e...
Microshift Test Compatibility ✅ Passed No new Ginkgo e2e tests were added in this PR. All test changes are limited to internal unit tests using Go's standard testing package, not Ginkgo e2e tests, so the MicroShift Test Compatibility ch...
Single Node Openshift (Sno) Test Compatibility ✅ Passed No Ginkgo e2e tests added in this PR. This PR implements platform filtering for oc-mirror using only standard Go unit tests, making the SNO compatibility check not applicable.
Topology-Aware Scheduling Compatibility ✅ Passed This PR modifies only the oc-mirror CLI tool's image platform filtering logic through Go source files. No Kubernetes deployment manifests, controllers, or scheduling constraints are introduced or m...
Ote Binary Stdout Contract ✅ Passed No process-level stdout writes or logging configuration violations found. All logging in test files is inside test functions, not in process-level code (main, init, BeforeSuite, AfterSuite, etc.)....
Ipv6 And Disconnected Network Test Compatibility ✅ Passed PR adds no new Ginkgo e2e tests. Only internal packages (14 files) and unit tests (3 files) modified; no changes to tests/integration or e2e test directories.
No-Weak-Crypto ✅ Passed PR implements platform filtering for multi-arch images using simple string operations and data structures; no MD5/SHA1/weak ciphers/custom crypto implementations or insecure comparisons are introdu...
Container-Privileges ✅ Passed PR adds platform filtering feature to Go CLI tool (oc-mirror); no Kubernetes manifests with privileged container configs, hostPID/hostNetwork/hostIPC, SYS_ADMIN, or allowPrivilegeEscalation were in...
No-Sensitive-Data-In-Logs ✅ Passed PR introduces no new logging of sensitive data. New code handles platform filtering (OS/architecture pairs) without logging values, and error messages only show format examples for user guidance.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: adding sparse manifest platform filtering for multi-arch images.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
internal/pkg/api/v2alpha1/type_config.go (1)

116-130: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Platform.DeepCopy() drops scalar fields and returns an incomplete copy.

Line 117 only seeds Graph; Release and KubeVirtContainer are lost in the copied struct. That can silently alter mirroring behavior when callers depend on DeepCopy().

Proposed fix
 func (p Platform) DeepCopy() Platform {
 	platformCopy := Platform{
-		Graph: p.Graph,
+		Graph:             p.Graph,
+		Release:           p.Release,
+		KubeVirtContainer: p.KubeVirtContainer,
 	}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/pkg/api/v2alpha1/type_config.go` around lines 116 - 130,
Platform.DeepCopy() currently initializes platformCopy with only Graph, causing
scalar fields like Release and KubeVirtContainer to be lost; update
Platform.DeepCopy to copy all scalar and value fields from the receiver (e.g.,
set platformCopy.Release = p.Release and platformCopy.KubeVirtContainer =
p.KubeVirtContainer) in addition to the existing deep copies of slices
(Channels, Architectures, Platforms) so the returned copy is complete; locate
the DeepCopy method on type Platform and add assignments for any other
scalar/value fields to mirror the source struct.
🧹 Nitpick comments (2)
internal/pkg/operator/filtered_collector_test.go (1)

1052-1061: ⚡ Quick win

Make the mock validate the forwarded platforms argument.

The new parameter is currently ignored, so these tests won’t detect broken propagation from operator config to catalog handler calls.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/pkg/operator/filtered_collector_test.go` around lines 1052 - 1061,
The mock getRelatedImagesFromCatalog in MockHandler ignores the new platforms
parameter; update it to validate the forwarded platforms pointer (the platforms
*[]string argument) before returning results — e.g., check platforms is not nil
and that *platforms matches the expected slice (or contains required entries)
and return an error (or fail) when it does not so tests detect propagation
failures; keep the existing relatedImages behavior otherwise and leave
copyImageSchemaMap untouched.
internal/pkg/operator/catalog_handler_test.go (1)

43-43: ⚡ Quick win

Add one non-nil platforms case to validate propagation in returned related images.

Current update only checks the nil path, so the new behavior can regress without test failure.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/pkg/operator/catalog_handler_test.go` at line 43, Add a test entry
in catalog_handler_test.go that calls handler.getRelatedImagesFromCatalog with a
non-nil platforms argument (e.g., a slice like []string{"linux/amd64"}) in
addition to the existing nil-case; call the same function used in the diff
(getRelatedImagesFromCatalog) with that non-nil platforms and
copyImageSchemaMap, assert err == nil, and verify that each returned related
image in res has its Platforms (or equivalent field) set to the same non-nil
slice (or contains the provided platforms), thereby validating propagation of
the platforms parameter through the function.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@go.mod`:
- Line 8: Update the pinned module version for
github.com/distribution/distribution/v3 in go.mod from v3.0.0 to a patched
release (>= v3.1.1) to address the listed GHSA advisories; after editing the
version string, run the appropriate Go module commands (e.g., go get
github.com/distribution/distribution/[email protected] and go mod tidy) to update go.sum
and ensure build/test pass, and verify no other transitive constraints (e.g.,
from github.com/containerd/containerd or github.com/moby/spdystream) prevent the
upgrade.
- Line 138: The project currently pins github.com/moby/spdystream v0.5.0; update
the dependency to v0.5.1+ by bumping the parent module that pulls it (or
directly requesting the newer version) so the vulnerable transitive dependency
is resolved, then run go mod tidy to refresh go.sum; locate references to
github.com/moby/spdystream in go.mod (and the parent module that requires it)
and run a go get or otherwise update that parent to force
github.com/moby/[email protected] (or later), followed by go mod tidy to update
go.sum.
- Line 63: Update the indirect dependency github.com/containerd/containerd from
v1.7.29 to at least v1.7.32 in go.mod to address GHSA-fqw6-gf59-qr4w; edit the
require directive for "github.com/containerd/containerd" to "v1.7.32" or a newer
compatible v1.7.x+ version, run "go get
github.com/containerd/[email protected]" (or higher) and then "go mod tidy" to
refresh go.sum and ensure the indirect upgrade is applied across the module
graph.

In `@internal/pkg/archive/image-blob-gatherer_test.go`:
- Line 293: The test currently hard-codes the full transport error string in the
assert.Equal call (the assert.Equal(t, "...", err.Error()) in
image-blob-gatherer_test.go), which is brittle across environments; change it to
use substring/contains assertions (e.g., assert.Contains or strings.Contains on
err.Error()) to check key phrases such as "error when creating a new image
source", "fetching manifest latest", and "connection refused" (or "pinging
container registry") instead of the full message so the test is robust to
platform-specific network wording.

In `@internal/pkg/mirror/mirror.go`:
- Around line 359-368: The loop that parses multiArch tokens (using variable
multiArch and splitting into parts) currently only checks len(parts)==2 and can
accept empty or whitespace OS/ARCH; update the validation in that block (the
code that builds copy.InstancePlatformFilter) to trim and reject empty parts and
enforce an allow-list pattern for OS and Architecture (e.g., only
letters/numbers, '-', '_' and dots) before appending to platforms; on invalid
tokens return the same error path (the fmt.Errorf currently returned alongside
copy.CopySystemImage) so malformed tokens are rejected at this trust boundary.

In `@internal/pkg/operator/filtered_collector.go`:
- Around line 251-255: The computed platforms slice is passed into
getRelatedImagesFromCatalog but not applied to the operator catalog image copy,
so the catalog copy ends up unfiltered; update the code that assembles or
creates the operator catalog image later in this function to accept and use the
platforms filter (e.g., pass the platforms variable into the catalog image
creation call or set the Platforms field on the catalog image struct before
invoking the copy), ensuring the same platforms value used with
o.ctlgHandler.getRelatedImagesFromCatalog(result.DeclConfig, platforms,
copyImageSchemaMap) is also applied to the operator catalog image copy.

In `@internal/pkg/release/local_stored_collector.go`:
- Around line 56-73: getPlatformFilters in LocalStorageCollector currently
returns nil when both o.Config.Mirror.Platform.Platforms and Architectures are
empty, causing "mirror all" behavior; change it to return a pointer to a slice
containing the default "linux/amd64" string instead. Specifically, in
getPlatformFilters (and where it falls back from Architectures), replace the
final "return nil" with creation of a []string{"linux/amd64"} and return &slice
so callers expecting *[]string receive the documented default platform.

---

Outside diff comments:
In `@internal/pkg/api/v2alpha1/type_config.go`:
- Around line 116-130: Platform.DeepCopy() currently initializes platformCopy
with only Graph, causing scalar fields like Release and KubeVirtContainer to be
lost; update Platform.DeepCopy to copy all scalar and value fields from the
receiver (e.g., set platformCopy.Release = p.Release and
platformCopy.KubeVirtContainer = p.KubeVirtContainer) in addition to the
existing deep copies of slices (Channels, Architectures, Platforms) so the
returned copy is complete; locate the DeepCopy method on type Platform and add
assignments for any other scalar/value fields to mirror the source struct.

---

Nitpick comments:
In `@internal/pkg/operator/catalog_handler_test.go`:
- Line 43: Add a test entry in catalog_handler_test.go that calls
handler.getRelatedImagesFromCatalog with a non-nil platforms argument (e.g., a
slice like []string{"linux/amd64"}) in addition to the existing nil-case; call
the same function used in the diff (getRelatedImagesFromCatalog) with that
non-nil platforms and copyImageSchemaMap, assert err == nil, and verify that
each returned related image in res has its Platforms (or equivalent field) set
to the same non-nil slice (or contains the provided platforms), thereby
validating propagation of the platforms parameter through the function.

In `@internal/pkg/operator/filtered_collector_test.go`:
- Around line 1052-1061: The mock getRelatedImagesFromCatalog in MockHandler
ignores the new platforms parameter; update it to validate the forwarded
platforms pointer (the platforms *[]string argument) before returning results —
e.g., check platforms is not nil and that *platforms matches the expected slice
(or contains required entries) and return an error (or fail) when it does not so
tests detect propagation failures; keep the existing relatedImages behavior
otherwise and leave copyImageSchemaMap untouched.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 1c8d1649-2c7f-439b-a8ae-62be25a36dee

📥 Commits

Reviewing files that changed from the base of the PR and between 192a13d and 9f7c059.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (17)
  • go.mod
  • internal/pkg/additional/local_stored_collector.go
  • internal/pkg/api/v2alpha1/type_config.go
  • internal/pkg/api/v2alpha1/type_internal.go
  • internal/pkg/api/v2alpha1/type_platform.go
  • internal/pkg/archive/image-blob-gatherer_test.go
  • internal/pkg/batch/concurrent_chan_worker.go
  • internal/pkg/helm/local_stored_collector.go
  • internal/pkg/mirror/mirror.go
  • internal/pkg/mirror/mirror_test.go
  • internal/pkg/mirror/options.go
  • internal/pkg/operator/catalog_handler.go
  • internal/pkg/operator/catalog_handler_test.go
  • internal/pkg/operator/filtered_collector.go
  • internal/pkg/operator/filtered_collector_test.go
  • internal/pkg/operator/interface.go
  • internal/pkg/release/local_stored_collector.go

Comment thread go.mod Outdated
Comment thread go.mod Outdated
Comment thread go.mod Outdated
Comment thread internal/pkg/archive/image-blob-gatherer_test.go Outdated
Comment on lines +359 to +368
for _, platform := range strings.Split(multiArch, ",") {
platform = strings.TrimSpace(platform)
parts := strings.Split(platform, "/")
if len(parts) != 2 {
return copy.CopySystemImage, nil, fmt.Errorf("unknown multi-arch option %q. Choose one of the supported options: 'system', 'all', 'index-only', or a comma-separated platform list like 'linux/amd64,linux/arm64'", multiArch)
}
platforms = append(platforms, copy.InstancePlatformFilter{
OS: parts[0],
Architecture: parts[1],
})

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Harden platform token validation before constructing filters.

Line 362 only validates len(parts) == 2; empty or whitespace OS/ARCH values still pass. Reject invalid tokens at this boundary to avoid propagating malformed platform filters.

Proposed fix
-		for _, platform := range strings.Split(multiArch, ",") {
+		for _, platform := range strings.Split(multiArch, ",") {
 			platform = strings.TrimSpace(platform)
-			parts := strings.Split(platform, "/")
-			if len(parts) != 2 {
+			parts := strings.SplitN(platform, "/", 2)
+			if len(parts) != 2 {
 				return copy.CopySystemImage, nil, fmt.Errorf("unknown multi-arch option %q. Choose one of the supported options: 'system', 'all', 'index-only', or a comma-separated platform list like 'linux/amd64,linux/arm64'", multiArch)
 			}
+			osPart := strings.TrimSpace(parts[0])
+			archPart := strings.TrimSpace(parts[1])
+			if osPart == "" || archPart == "" {
+				return copy.CopySystemImage, nil, fmt.Errorf("invalid platform %q: expected non-empty os/architecture", platform)
+			}
 			platforms = append(platforms, copy.InstancePlatformFilter{
-				OS:           parts[0],
-				Architecture: parts[1],
+				OS:           osPart,
+				Architecture: archPart,
 			})
 		}

As per coding guidelines, “Validate at trust boundaries with allow-lists, not deny-lists.”

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
for _, platform := range strings.Split(multiArch, ",") {
platform = strings.TrimSpace(platform)
parts := strings.Split(platform, "/")
if len(parts) != 2 {
return copy.CopySystemImage, nil, fmt.Errorf("unknown multi-arch option %q. Choose one of the supported options: 'system', 'all', 'index-only', or a comma-separated platform list like 'linux/amd64,linux/arm64'", multiArch)
}
platforms = append(platforms, copy.InstancePlatformFilter{
OS: parts[0],
Architecture: parts[1],
})
for _, platform := range strings.Split(multiArch, ",") {
platform = strings.TrimSpace(platform)
parts := strings.SplitN(platform, "/", 2)
if len(parts) != 2 {
return copy.CopySystemImage, nil, fmt.Errorf("unknown multi-arch option %q. Choose one of the supported options: 'system', 'all', 'index-only', or a comma-separated platform list like 'linux/amd64,linux/arm64'", multiArch)
}
osPart := strings.TrimSpace(parts[0])
archPart := strings.TrimSpace(parts[1])
if osPart == "" || archPart == "" {
return copy.CopySystemImage, nil, fmt.Errorf("invalid platform %q: expected non-empty os/architecture", platform)
}
platforms = append(platforms, copy.InstancePlatformFilter{
OS: osPart,
Architecture: archPart,
})
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/pkg/mirror/mirror.go` around lines 359 - 368, The loop that parses
multiArch tokens (using variable multiArch and splitting into parts) currently
only checks len(parts)==2 and can accept empty or whitespace OS/ARCH; update the
validation in that block (the code that builds copy.InstancePlatformFilter) to
trim and reject empty parts and enforce an allow-list pattern for OS and
Architecture (e.g., only letters/numbers, '-', '_' and dots) before appending to
platforms; on invalid tokens return the same error path (the fmt.Errorf
currently returned alongside copy.CopySystemImage) so malformed tokens are
rejected at this trust boundary.

Source: Coding guidelines

Comment thread internal/pkg/operator/filtered_collector.go Outdated
Comment on lines +56 to +73
func (o LocalStorageCollector) getPlatformFilters() *[]string {
// Use new Platforms field if available
if len(o.Config.Mirror.Platform.Platforms) > 0 {
return v2alpha1.ConvertPlatformsToStringSlice(o.Config.Mirror.Platform.Platforms)
}

// Fall back to deprecated Architectures field (assume linux)
if len(o.Config.Mirror.Platform.Architectures) > 0 {
platformStrs := make([]string, len(o.Config.Mirror.Platform.Architectures))
for i, arch := range o.Config.Mirror.Platform.Architectures {
platformStrs[i] = "linux/" + arch
}
return &platformStrs
}

// No platforms specified - mirror all
return nil
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Default platform behavior conflicts with config contract.

When both Platforms and Architectures are empty, Line 72 returns nil (mirror all). But Platform docs declare the default should be linux/amd64. This mismatch can unexpectedly mirror every platform.

Proposed fix
 func (o LocalStorageCollector) getPlatformFilters() *[]string {
@@
 	// No platforms specified - mirror all
-	return nil
+	defaultPlatform := []string{"linux/" + v2alpha1.DefaultPlatformArchitecture}
+	return &defaultPlatform
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (o LocalStorageCollector) getPlatformFilters() *[]string {
// Use new Platforms field if available
if len(o.Config.Mirror.Platform.Platforms) > 0 {
return v2alpha1.ConvertPlatformsToStringSlice(o.Config.Mirror.Platform.Platforms)
}
// Fall back to deprecated Architectures field (assume linux)
if len(o.Config.Mirror.Platform.Architectures) > 0 {
platformStrs := make([]string, len(o.Config.Mirror.Platform.Architectures))
for i, arch := range o.Config.Mirror.Platform.Architectures {
platformStrs[i] = "linux/" + arch
}
return &platformStrs
}
// No platforms specified - mirror all
return nil
}
func (o LocalStorageCollector) getPlatformFilters() *[]string {
// Use new Platforms field if available
if len(o.Config.Mirror.Platform.Platforms) > 0 {
return v2alpha1.ConvertPlatformsToStringSlice(o.Config.Mirror.Platform.Platforms)
}
// Fall back to deprecated Architectures field (assume linux)
if len(o.Config.Mirror.Platform.Architectures) > 0 {
platformStrs := make([]string, len(o.Config.Mirror.Platform.Architectures))
for i, arch := range o.Config.Mirror.Platform.Architectures {
platformStrs[i] = "linux/" + arch
}
return &platformStrs
}
// No platforms specified - mirror all
defaultPlatform := []string{"linux/" + v2alpha1.DefaultPlatformArchitecture}
return &defaultPlatform
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/pkg/release/local_stored_collector.go` around lines 56 - 73,
getPlatformFilters in LocalStorageCollector currently returns nil when both
o.Config.Mirror.Platform.Platforms and Architectures are empty, causing "mirror
all" behavior; change it to return a pointer to a slice containing the default
"linux/amd64" string instead. Specifically, in getPlatformFilters (and where it
falls back from Architectures), replace the final "return nil" with creation of
a []string{"linux/amd64"} and return &slice so callers expecting *[]string
receive the documented default platform.

@aguidirh
aguidirh force-pushed the CLID-290 branch 2 times, most recently from 3219c3e to 74ce905 Compare June 8, 2026 15:46

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
internal/pkg/mirror/mirror.go (1)

368-377: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Strengthen platform token validation at this trust boundary.

The current validation only checks len(parts) == 2 and doesn't validate the content of OS/Architecture tokens after splitting. Empty or whitespace-only values (e.g., " / ", "linux/ ") can pass validation and create malformed platform filters. As per coding guidelines, input validation at trust boundaries should use allow-lists, not just structural checks.

🛡️ Proposed fix to validate and sanitize platform tokens
 		for _, platform := range strings.Split(multiArch, ",") {
 			platform = strings.TrimSpace(platform)
-			parts := strings.Split(platform, "/")
+			parts := strings.SplitN(platform, "/", 2)
 			if len(parts) != 2 {
 				return copy.CopySystemImage, nil, fmt.Errorf("unknown multi-arch option %q. Choose one of the supported options: 'system', 'all', 'index-only', or a comma-separated platform list like 'linux/amd64,linux/arm64'", multiArch)
 			}
+			osPart := strings.TrimSpace(parts[0])
+			archPart := strings.TrimSpace(parts[1])
+			if osPart == "" || archPart == "" {
+				return copy.CopySystemImage, nil, fmt.Errorf("invalid platform %q: OS and architecture must be non-empty", platform)
+			}
+			// Validate against allow-list: alphanumeric, hyphen, underscore, dot
+			validPattern := regexp.MustCompile(`^[a-zA-Z0-9._-]+$`)
+			if !validPattern.MatchString(osPart) || !validPattern.MatchString(archPart) {
+				return copy.CopySystemImage, nil, fmt.Errorf("invalid platform %q: OS and architecture must contain only letters, numbers, hyphens, underscores, and dots", platform)
+			}
 			platforms = append(platforms, copy.InstancePlatformFilter{
-				OS:           parts[0],
-				Architecture: parts[1],
+				OS:           osPart,
+				Architecture: archPart,
 			})
 		}

Note: You'll need to import "regexp" at the top of the file.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/pkg/mirror/mirror.go` around lines 368 - 377, The loop that parses
multiArch platforms accepts tokens with only a structural check (len(parts)==2)
which allows empty or whitespace OS/Architecture values; update the logic in the
function containing this loop (the multi-arch platform parsing block in
mirror.go) to (1) Trim and validate each token and each part after splitting,
(2) enforce an allow-list/regex for OS and Architecture (e.g., permit only
lowercase letters, digits and hyphens/underscores) and reject empty strings, and
(3) return a clear error when a token fails validation; add the required import
("regexp") and use the validated parts when building copy.InstancePlatformFilter
to prevent malformed platform filters.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Duplicate comments:
In `@internal/pkg/mirror/mirror.go`:
- Around line 368-377: The loop that parses multiArch platforms accepts tokens
with only a structural check (len(parts)==2) which allows empty or whitespace
OS/Architecture values; update the logic in the function containing this loop
(the multi-arch platform parsing block in mirror.go) to (1) Trim and validate
each token and each part after splitting, (2) enforce an allow-list/regex for OS
and Architecture (e.g., permit only lowercase letters, digits and
hyphens/underscores) and reject empty strings, and (3) return a clear error when
a token fails validation; add the required import ("regexp") and use the
validated parts when building copy.InstancePlatformFilter to prevent malformed
platform filters.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 6601800d-c8a5-472a-8855-b3444425902e

📥 Commits

Reviewing files that changed from the base of the PR and between 9f7c059 and 3219c3e.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (17)
  • go.mod
  • internal/pkg/additional/local_stored_collector.go
  • internal/pkg/api/v2alpha1/type_config.go
  • internal/pkg/api/v2alpha1/type_internal.go
  • internal/pkg/api/v2alpha1/type_platform.go
  • internal/pkg/archive/image-blob-gatherer_test.go
  • internal/pkg/batch/concurrent_chan_worker.go
  • internal/pkg/helm/local_stored_collector.go
  • internal/pkg/mirror/mirror.go
  • internal/pkg/mirror/mirror_test.go
  • internal/pkg/mirror/options.go
  • internal/pkg/operator/catalog_handler.go
  • internal/pkg/operator/catalog_handler_test.go
  • internal/pkg/operator/filtered_collector.go
  • internal/pkg/operator/filtered_collector_test.go
  • internal/pkg/operator/interface.go
  • internal/pkg/release/local_stored_collector.go
✅ Files skipped from review due to trivial changes (1)
  • internal/pkg/archive/image-blob-gatherer_test.go
🚧 Files skipped from review as they are similar to previous changes (12)
  • internal/pkg/operator/filtered_collector_test.go
  • internal/pkg/operator/interface.go
  • internal/pkg/operator/catalog_handler.go
  • internal/pkg/api/v2alpha1/type_platform.go
  • internal/pkg/operator/catalog_handler_test.go
  • internal/pkg/batch/concurrent_chan_worker.go
  • internal/pkg/additional/local_stored_collector.go
  • internal/pkg/operator/filtered_collector.go
  • internal/pkg/helm/local_stored_collector.go
  • internal/pkg/api/v2alpha1/type_internal.go
  • internal/pkg/api/v2alpha1/type_config.go
  • internal/pkg/release/local_stored_collector.go

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
go.mod (1)

8-8: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Resolve known vulnerable pinned modules before merge.

go.mod still pins versions flagged as HIGH severity:

  • Line 8: github.com/distribution/distribution/v3 v3.0.0
  • Line 63: github.com/containerd/containerd v1.7.29
  • Line 138: github.com/moby/spdystream v0.5.0

Please bump to patched releases and refresh go.sum (go get ... && go mod tidy) to satisfy the repo’s dependency security policy.

#!/bin/bash
set -euo pipefail

echo "== Current versions in go.mod =="
rg -n 'github.com/distribution/distribution/v3|github.com/containerd/containerd|github.com/moby/spdystream' go.mod

echo
echo "== OSV check: distribution/v3 =="
curl -s https://api.osv.dev/v1/query -H 'Content-Type: application/json' \
  -d '{"package":{"ecosystem":"Go","name":"github.com/distribution/distribution/v3"}}' \
  | jq '.vulns[]? | {id: .id, summary: .summary, affected: .affected[0].ranges}'

echo
echo "== OSV check: containerd =="
curl -s https://api.osv.dev/v1/query -H 'Content-Type: application/json' \
  -d '{"package":{"ecosystem":"Go","name":"github.com/containerd/containerd"}}' \
  | jq '.vulns[]? | {id: .id, summary: .summary, affected: .affected[0].ranges}'

echo
echo "== OSV check: moby/spdystream =="
curl -s https://api.osv.dev/v1/query -H 'Content-Type: application/json' \
  -d '{"package":{"ecosystem":"Go","name":"github.com/moby/spdystream"}}' \
  | jq '.vulns[]? | {id: .id, summary: .summary, affected: .affected[0].ranges}'

Also applies to: 63-63, 138-138

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@go.mod` at line 8, go.mod pins vulnerable module versions for
github.com/distribution/distribution/v3, github.com/containerd/containerd and
github.com/moby/spdystream; update each to the latest patched release, run “go
get github.com/distribution/distribution/v3@<patched>
github.com/containerd/containerd@<patched> github.com/moby/spdystream@<patched>”
(replace <patched> with the secure versions) and then run “go mod tidy” to
refresh go.sum; ensure the updated module lines
(github.com/distribution/distribution/v3, github.com/containerd/containerd,
github.com/moby/spdystream) in go.mod reflect the new versions and commit the
resulting go.mod and go.sum changes.

Sources: Coding guidelines, Linters/SAST tools

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Duplicate comments:
In `@go.mod`:
- Line 8: go.mod pins vulnerable module versions for
github.com/distribution/distribution/v3, github.com/containerd/containerd and
github.com/moby/spdystream; update each to the latest patched release, run “go
get github.com/distribution/distribution/v3@<patched>
github.com/containerd/containerd@<patched> github.com/moby/spdystream@<patched>”
(replace <patched> with the secure versions) and then run “go mod tidy” to
refresh go.sum; ensure the updated module lines
(github.com/distribution/distribution/v3, github.com/containerd/containerd,
github.com/moby/spdystream) in go.mod reflect the new versions and commit the
resulting go.mod and go.sum changes.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: fc19da77-8286-4674-9b7f-072db74d31b2

📥 Commits

Reviewing files that changed from the base of the PR and between 3219c3e and 74ce905.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (17)
  • go.mod
  • internal/pkg/additional/local_stored_collector.go
  • internal/pkg/api/v2alpha1/type_config.go
  • internal/pkg/api/v2alpha1/type_internal.go
  • internal/pkg/api/v2alpha1/type_platform.go
  • internal/pkg/archive/image-blob-gatherer_test.go
  • internal/pkg/batch/concurrent_chan_worker.go
  • internal/pkg/helm/local_stored_collector.go
  • internal/pkg/mirror/mirror.go
  • internal/pkg/mirror/mirror_test.go
  • internal/pkg/mirror/options.go
  • internal/pkg/operator/catalog_handler.go
  • internal/pkg/operator/catalog_handler_test.go
  • internal/pkg/operator/filtered_collector.go
  • internal/pkg/operator/filtered_collector_test.go
  • internal/pkg/operator/interface.go
  • internal/pkg/release/local_stored_collector.go
✅ Files skipped from review due to trivial changes (1)
  • internal/pkg/archive/image-blob-gatherer_test.go
🚧 Files skipped from review as they are similar to previous changes (15)
  • internal/pkg/operator/catalog_handler_test.go
  • internal/pkg/operator/interface.go
  • internal/pkg/mirror/options.go
  • internal/pkg/operator/filtered_collector.go
  • internal/pkg/additional/local_stored_collector.go
  • internal/pkg/batch/concurrent_chan_worker.go
  • internal/pkg/mirror/mirror_test.go
  • internal/pkg/operator/filtered_collector_test.go
  • internal/pkg/api/v2alpha1/type_platform.go
  • internal/pkg/api/v2alpha1/type_config.go
  • internal/pkg/release/local_stored_collector.go
  • internal/pkg/helm/local_stored_collector.go
  • internal/pkg/api/v2alpha1/type_internal.go
  • internal/pkg/operator/catalog_handler.go
  • internal/pkg/mirror/mirror.go

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
internal/pkg/operator/filtered_collector.go (1)

251-255: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Catalog image misses platform filter propagation

Line 306 builds the catalog RelatedImage without Platforms, so the catalog copy path can still mirror all architectures even though Line 254 now filters related images.

Suggested patch
 	relatedImages[componentName] = []v2alpha1.RelatedImage{
 		{
 			Name:          catalogName,
 			Image:         catalogImage,
 			Type:          v2alpha1.TypeOperatorCatalog,
 			TargetTag:     targetTag,
 			TargetCatalog: op.TargetCatalog,
 			RebuiltTag:    rebuiltTag,
 			FullCatalog:   isFullCatalog(op),
+			Platforms:     platforms,
 		},
 	}

Also applies to: 306-315

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/pkg/operator/filtered_collector.go` around lines 251 - 255, The
catalog-related image construction is missing propagation of platform filters:
after you call o.ctlgHandler.getRelatedImagesFromCatalog(result.DeclConfig,
platforms, copyImageSchemaMap) (where platforms :=
v2alpha1.ConvertPlatformsToStringSlice(op.Platforms)), ensure that each
RelatedImage created for the catalog copy path includes the same Platforms value
(set RelatedImage.Platforms = platforms or equivalent) so the catalog copy
honors op.Platforms; update the code paths that build RelatedImage instances
(the block referenced around getRelatedImagesFromCatalog and the construction in
the 306–315 region) to copy/set the platforms slice from the platforms variable
into the RelatedImage before returning or appending.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@go.mod`:
- Line 3: The go.mod specifies Go 1.25.6 but the build sandbox uses
GO_VERSION=go1.23.5 causing mismatched toolchains; update the hack/build.sh,
hack/e2e.sh and the Dockerfile (local/go-toolset) to either install/use Go
1.25.6 (set GO_VERSION=go1.25.6 and ensure the local/go-toolset image installs
that toolchain) or make the scripts export GOTOOLCHAIN=auto so the runtime
respects the go.mod directive; change occurrences of GO_VERSION and any image
build steps that pull the older toolset, and verify createToolchain/install
steps in local/go-toolset use the 1.25.6 installer to keep builds consistent
with go.mod.

---

Duplicate comments:
In `@internal/pkg/operator/filtered_collector.go`:
- Around line 251-255: The catalog-related image construction is missing
propagation of platform filters: after you call
o.ctlgHandler.getRelatedImagesFromCatalog(result.DeclConfig, platforms,
copyImageSchemaMap) (where platforms :=
v2alpha1.ConvertPlatformsToStringSlice(op.Platforms)), ensure that each
RelatedImage created for the catalog copy path includes the same Platforms value
(set RelatedImage.Platforms = platforms or equivalent) so the catalog copy
honors op.Platforms; update the code paths that build RelatedImage instances
(the block referenced around getRelatedImagesFromCatalog and the construction in
the 306–315 region) to copy/set the platforms slice from the platforms variable
into the RelatedImage before returning or appending.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 3d59dc56-7672-41f8-a6be-eaf1d8c7f04a

📥 Commits

Reviewing files that changed from the base of the PR and between 74ce905 and 9074ce3.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (17)
  • go.mod
  • internal/pkg/additional/local_stored_collector.go
  • internal/pkg/api/v2alpha1/type_config.go
  • internal/pkg/api/v2alpha1/type_internal.go
  • internal/pkg/api/v2alpha1/type_platform.go
  • internal/pkg/archive/image-blob-gatherer_test.go
  • internal/pkg/batch/concurrent_chan_worker.go
  • internal/pkg/helm/local_stored_collector.go
  • internal/pkg/mirror/mirror.go
  • internal/pkg/mirror/mirror_test.go
  • internal/pkg/mirror/options.go
  • internal/pkg/operator/catalog_handler.go
  • internal/pkg/operator/catalog_handler_test.go
  • internal/pkg/operator/filtered_collector.go
  • internal/pkg/operator/filtered_collector_test.go
  • internal/pkg/operator/interface.go
  • internal/pkg/release/local_stored_collector.go
🚧 Files skipped from review as they are similar to previous changes (13)
  • internal/pkg/mirror/options.go
  • internal/pkg/archive/image-blob-gatherer_test.go
  • internal/pkg/additional/local_stored_collector.go
  • internal/pkg/batch/concurrent_chan_worker.go
  • internal/pkg/operator/catalog_handler_test.go
  • internal/pkg/api/v2alpha1/type_platform.go
  • internal/pkg/api/v2alpha1/type_internal.go
  • internal/pkg/operator/interface.go
  • internal/pkg/mirror/mirror_test.go
  • internal/pkg/operator/catalog_handler.go
  • internal/pkg/release/local_stored_collector.go
  • internal/pkg/mirror/mirror.go
  • internal/pkg/api/v2alpha1/type_config.go

Comment thread go.mod Outdated

@adolfo-ab adolfo-ab left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check if you need to also update internal/pkg/config/defaults.go and cincinnati.go?

Also, maybe you could add one or two integration tests, so that this feature is already covered.


// Fall back to deprecated Architectures field (assume linux)
//nolint:staticcheck // SA1019: Architectures is deprecated but we maintain backward compatibility
if len(o.Config.Mirror.Platform.Architectures) > 0 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since Architectures is deprecated, maybe we can add a log line mentioning it here, and asking to consider using Platform instead

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed at 8c26134

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please mark it as resolved in case there is no further suggestions for this thread.

@adolfo-ab adolfo-ab Jul 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It won't let me resolve this one, but feel free to do it yourself in you can or just move on as if it was

// InstancePlatformFilter defines OS and Architecture for filtering
// multi-architecture manifest lists. This allows selecting specific
// platforms (e.g., linux/amd64, linux/arm64) when mirroring images.
type InstancePlatformFilter struct {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if a user only sets the OS or the Architecture? Should we check that we always have both? Otherwise we might get linux/ or /amd64, no?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed at 8c26134

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please mark it as resolved in case there is no further suggestions for this thread.

@aguidirh

Copy link
Copy Markdown
Contributor Author

/test v1-e2e

aguidirh added 5 commits July 8, 2026 15:48
Implement InstancePlatformFilter feature with sparse manifest support to
allow users to specify OS/Architecture pairs for filtering multi-architecture
images across all image types (additional images, operators, helm charts,
and releases).

This feature brings the platform filtering capability from skopeo
(containers/image PR #2874) to oc-mirror, enabling users to mirror only
specific platforms instead of all architectures. The implementation uses
sparse manifests where the manifest list references all platforms but only
the specified platforms have their actual layer blobs downloaded, significantly
reducing disk usage and bandwidth.

Platform filtering is applied during Mirror-to-Disk and Mirror-to-Mirror
workflows. Disk-to-Mirror workflow uses already-filtered images from disk
without re-applying platform filters.

The implementation includes backward compatibility with the deprecated
Platform.Architectures field and follows the same strategy as skopeo
for consistency.

Fixes: CLID-290
Signed-off-by: Alex Guidi <[email protected]>
…ages

refactor: Replace per-image Platforms with CollectorSchema.PlatformFilters map

This is a cleaner approach than the previous commit. Instead of storing
platform filters on RelatedImage and CopyImageSchema (polluting image
structs with filtering concerns), each collector now populates a
PlatformFilters map[string][]string on CollectorSchema, keyed by image
origin. The batch worker reads from this map at copy time.

Note: platform filtering for release content images is not yet functional
end-to-end. For it to work, the Cincinnati query must return the multi-arch
payload (arch=multi) so that image-references contains manifest list digests
rather than single-arch digests. This is tracked as a follow-up.

Please review the TBD comments in the code — particularly in executor.go
(cross-collector platform merge semantics) and filtered_collector.go
(localRelatedImages approach) — as these decisions benefit from team input.
…ages

fix: Address code review suggestions from adolfo-ab and coderabbitai

- Add Validate() to InstancePlatformFilter to reject entries with empty
  OS or Architecture, preventing malformed strings like "linux/" or "/amd64"
- Update ConvertPlatformsToStringSlice to skip invalid entries rather than
  producing malformed platform strings
- Add deprecation warning log when the Architectures field is used, directing
  users to switch to the new platform.platforms field
…ages

fix: Reduce cyclomatic complexity in collector functions to pass linter

Extract helpers to bring three functions within the cyclop threshold (max 10):
- ReleaseImageCollector: extract compareByOriginSourceDest sort helper
- AdditionalImagesCollector: extract resolveAdditionalImageSrcDest and
  resolveTargetRepoTag helpers
- HelmImageCollector: extract collectHelmImagesM2D and collectHelmImagesD2M
  helpers, reducing the main function to a simple mode dispatch
…ages

fix: Enable sparse manifest support in the local cache registry

Add validation.manifests.indexes.platforms: none to the local cache
registry configuration so it matches the behavior of a target registry
configured for sparse manifests. Without this setting, the distribution
registry validates that all blobs referenced in a manifest list are present
before accepting the push, causing "blob unknown to registry" errors when
platform filtering leaves some architecture blobs uncopied.
…ages

Refactor release platform handling to preserve backward compatibility while
enabling sparse manifest filtering for users who adopt the new Platforms field.

Cincinnati GetReleaseReferenceImages now branches on Platform.Platforms vs
Platform.Architectures:
- Platforms set: always queries Cincinnati with ?arch=multi (the only manifest
  list payload) and warns that the target registry must support sparse manifest
  lists (validation.manifests.indexes.platforms: none).
- Architectures set (or defaulted to ["amd64"] by Complete()): preserves the
  original per-arch Cincinnati loop unchanged.

The two fields are mutually exclusive — a new validateReleasePlatformFields
check in Validate() returns an error if both are specified.

defaults.go no longer auto-fills Architectures when Platforms is already set,
preventing spurious defaults from being added alongside an explicit Platforms
configuration.

ReleaseImageCollector sets MultiArch="system" by default (original behavior)
and overrides to "all" only when Platforms is configured, so that
ensureReleaseInOCIFormat downloads all arch payloads from the multi-arch
release payload and can read image-references with manifest-list component
digests.

RemoveSignatures=true is restored in ensureReleaseInOCIFormat: converting
docker to OCI format changes the image digest, which invalidates any attached
signatures. Since this copy is for metadata extraction only, signatures are not
needed and keeping them would cause downstream failures.
@aguidirh

aguidirh commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Next round:

  • Confirm the feature is working by running some manual tests.
  • Implement an integration test to cover the sparse manifest feature on all types on both m2m and m2d/d2m workflows.

…ages

Simplify getPlatformFilters(): the Architectures field is only for Cincinnati
(?arch=<arch>), so the batch worker does not need InstancePlatforms for that
path. Returning nil for the Architectures case preserves the original behavior
where per-arch single-arch payloads are copied whole without platform filtering.

Move the Architectures deprecation warning from getPlatformFilters() to the
Cincinnati Architectures branch in GetReleaseReferenceImages, where the field
is actually consumed.
@aguidirh

Copy link
Copy Markdown
Contributor Author

/retest

…ages

Fix archive creation failing for M2D when platform filtering is active.

When sparse manifest filtering is used, the local cache registry stores a
complete manifest list index but only the blobs for the selected platforms.
The archive step calls ImageManifest for every digest in the manifest list,
including platforms that were never mirrored, which returns "manifest unknown"
from the registry and caused the archive build to abort.

Fix: skip absent platform manifests in multiArchBlobs with the same
"manifest unknown" check already used in mirror.go, logging a debug message
and continuing to the next platform.
…ages

Fix M2D archive creation failing when sparse manifest platform filtering is
active. The archive step iterates over every digest in a release manifest list
to collect the blobs that need to be packaged. With sparse filtering, the local
cache only stores blobs for the configured platforms — other platforms are absent.

Two bugs caused the archive build to fail:
1. ImageManifest returned "manifest unknown" for absent platform digests, which
   was treated as a fatal error instead of an expected condition.
2. The absent platform digest was inserted into the blobs set before the manifest
   check, so the archive later tried to locate a blob file that was never stored.

Fix: skip absent platform digests that are not in the image's allowed platform
set (CollectorSchema.PlatformFilters[img.Origin]). The digest is only added to
the blobs set after a successful manifest fetch. Platforms that are in the allowed
set but return "manifest unknown" still fail — that indicates a genuine error.

To make PlatformFilters available to the archive builder without a separate setter
or interface change, BuildArchive now accepts a CollectorSchema instead of a plain
image slice, and the batch worker propagates PlatformFilters into the returned
CollectorSchema.
@aguidirh

Copy link
Copy Markdown
Contributor Author

/retest

@aguidirh aguidirh changed the title WIP: CLID-290: feat: Add sparse manifest platform filtering for multi-arch images CLID-290: feat: Add sparse manifest platform filtering for multi-arch images Jul 15, 2026
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 15, 2026
@aguidirh

Copy link
Copy Markdown
Contributor Author

/hold

I will unhold it as soon as I send the integration tests commit.

@openshift-ci openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 15, 2026

@adolfo-ab adolfo-ab left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, if I understand correctly, if a user writes a platform that doesn't exist in the ISC, we always continue the mirroring process.

In the case that the image is a manifest list but doesn't contain the architecture defined in the ISC, we retry ignoring the platform filter. In other cases (typo in the platform, or the image being single-arch) we just ignore the filter without warning.

IMHO, if the user defines a platform in the ISC, we can assume that he wants that specific architecture and nothing else, and if that platform doesn't exist for that image for whatever reason, we should just fail for that image, print a warning, and continue with the next image in the ISC, instead of retrying without filter or ignoring the filter altogether.

Wdyt @aguidirh ?

collectorSchema.CopyImageSchemaMap = operatorImgs.CopyImageSchemaMap
collectorSchema.CatalogToFBCMap = operatorImgs.CatalogToFBCMap

// TODO: if images are shared between types, maps.Copy will override platform filters for

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can perform this merge in removeDuplicatedImages, which is called once per collector in CollectAll. That way we can have Platforms in CopyImageSchema, populated by each collector and we don't need the PlatformFilter map in CollectorSchema. I think this would simplify the PR, although it's similar to your first approach so maybe I'm missing something here.


// Priority 2: Fall back to MultiArch flag (for CLI direct usage or default "all")
if len(opts.MultiArch) > 0 && opts.All {
return copy.CopySystemImage, nil, fmt.Errorf("cannot use --all and --multi-arch flags together")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These error messages make it seem like oc-mirror had --all and --multi-arch flags, which might confuse the user, since these are skopeo CLI flags.

…ages

Add integration tests and multi-arch release image builder infrastructure:
- sparse_manifest_test.go: 10 Ginkgo specs covering all 7 manual test scenarios
  (M2M filtered, M2M backward compat, M2D+D2M filtered, M2D+D2M backward compat,
  config validation, deprecated architectures + release channel, negative test)
- sparse_manifest_helpers_test.go: test helpers for platform blob presence assertions
- ISC fixtures: 8 image set config files for the sparse manifest test scenarios
- registry-config-sparse.yaml: distribution registry config with platforms:none
- create-release-multi.sh: build and push the 4-arch fake OCP release manifest list
- generate-release-signature-multi.sh: GPG-sign both single and multi-arch releases
  with a shared key so a single release-pk.asc validates all signatures
- Per-arch OCI image config blobs (arm64, s390x, ppc64le) alongside existing amd64
- config-multi.json: 4-placeholder manifest template for multi-arch builds
- release-payload-multi/index.json + oci-layout: committed OCI manifest list
- Makefile: add sign-multi cosign target
- testdata/keys: updated GPG public key and signatures for both release images
- helpers_test.go: copy all signature files in setupWorkDir (not just one)
@openshift-ci

openshift-ci Bot commented Jul 17, 2026

Copy link
Copy Markdown

@aguidirh: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/sanity 9ee5963 link true /test sanity
ci/prow/integration 9ee5963 link true /test integration

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants